-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[OptBisect] Add support for selecting ranges of passes and refactor DebugCounter to use a shared Range API. #152393
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-llvm-support @llvm/pr-subscribers-llvm-ir Author: Yonah Goldberg (YonahGoldberg) ChangesAdd -opt-bisect-skip=<idx> option for skipping pass at index <idx> in the pipeline. OptBisect allows you to binary search for a pass causing some error. It is useful as a next step to disable that pass as a sanity check and to see if the issue is isolated to one pass. Full diff: https://github.com/llvm/llvm-project/pull/152393.diff 3 Files Affected:
diff --git a/llvm/include/llvm/IR/OptBisect.h b/llvm/include/llvm/IR/OptBisect.h
index d813ae933d65e..9dcc0077552d4 100644
--- a/llvm/include/llvm/IR/OptBisect.h
+++ b/llvm/include/llvm/IR/OptBisect.h
@@ -14,6 +14,7 @@
#ifndef LLVM_IR_OPTBISECT_H
#define LLVM_IR_OPTBISECT_H
+#include "llvm/ADT/SmallSet.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/StringSet.h"
#include "llvm/Support/Compiler.h"
@@ -67,7 +68,13 @@ class LLVM_ABI OptBisect : public OptPassGate {
StringRef IRDescription) const override;
/// isEnabled() should return true before calling shouldRunPass().
- bool isEnabled() const override { return BisectLimit != Disabled; }
+ bool isEnabled() const override {
+ return BisectLimit != Disabled || !BisectSkipNumbers.empty();
+ }
+
+ /// Add pass at index SkipNumber to the list of passes to skip
+ /// during bisection.
+ void addSkip(int SkipNumber) { BisectSkipNumbers.insert(SkipNumber); }
/// Set the new optimization limit and reset the counter. Passing
/// OptBisect::Disabled disables the limiting.
@@ -81,6 +88,7 @@ class LLVM_ABI OptBisect : public OptPassGate {
private:
int BisectLimit = Disabled;
mutable int LastBisectNum = 0;
+ SmallSet<int, 4> BisectSkipNumbers;
};
/// This class implements a mechanism to disable passes and individual
diff --git a/llvm/lib/IR/OptBisect.cpp b/llvm/lib/IR/OptBisect.cpp
index 29ca268408265..defb8a98f9716 100644
--- a/llvm/lib/IR/OptBisect.cpp
+++ b/llvm/lib/IR/OptBisect.cpp
@@ -37,6 +37,11 @@ static cl::opt<int> OptBisectLimit("opt-bisect-limit", cl::Hidden,
}),
cl::desc("Maximum optimization to perform"));
+static cl::opt<int> OptBisectSkip(
+ "opt-bisect-skip", cl::Hidden, cl::init(OptBisect::Disabled), cl::Optional,
+ cl::cb<void, int>([](int PassNum) { getOptBisector().addSkip(PassNum); }),
+ cl::desc("Skip pass at the given index in the optimization pipeline"));
+
static cl::opt<bool> OptBisectVerbose(
"opt-bisect-verbose",
cl::desc("Show verbose output when opt-bisect-limit is set"), cl::Hidden,
@@ -66,7 +71,9 @@ bool OptBisect::shouldRunPass(StringRef PassName,
assert(isEnabled());
int CurBisectNum = ++LastBisectNum;
- bool ShouldRun = (BisectLimit == -1 || CurBisectNum <= BisectLimit);
+ bool ShouldRun = (BisectLimit == -1 || BisectLimit == Disabled ||
+ CurBisectNum <= BisectLimit) &&
+ !BisectSkipNumbers.contains(CurBisectNum);
if (OptBisectVerbose)
printPassMessage(PassName, CurBisectNum, IRDescription, ShouldRun);
return ShouldRun;
diff --git a/llvm/test/Other/opt-bisect-skip.ll b/llvm/test/Other/opt-bisect-skip.ll
new file mode 100644
index 0000000000000..da94ad6230527
--- /dev/null
+++ b/llvm/test/Other/opt-bisect-skip.ll
@@ -0,0 +1,15 @@
+; Test that verifies functionality for -opt-bisect-skip
+
+; RUN: opt -O1 -opt-bisect-skip=3 -opt-bisect-skip=7 %s 2>&1 | FileCheck %s --check-prefix=CHECK-DISABLE-PASS
+; CHECK-DISABLE-PASS: BISECT: running pass (1) annotation2metadata on [module]
+; CHECK-DISABLE-PASS: BISECT: running pass (2) forceattrs on [module]
+; CHECK-DISABLE-PASS: BISECT: NOT running pass (3) inferattrs on [module]
+; CHECK-DISABLE-PASS: BISECT: running pass (4) lower-expect on foo
+; CHECK-DISABLE-PASS: BISECT: running pass (5) simplifycfg on foo
+; CHECK-DISABLE-PASS: BISECT: running pass (6) sroa on foo
+; CHECK-DISABLE-PASS: BISECT: NOT running pass (7) early-cse on foo
+; CHECK-DISABLE-PASS: BISECT: running pass (8) openmp-opt on [module]
+
+define void @foo() {
+ ret void
+}
\ No newline at end of file
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I personally use this feature in our down-stream fork and find it to be a nice complement to --opt-bisect-limit when investigating correctness issues. Please wait for some more review prior to landing.
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to make opt-bisect work the same way that debug-counter works nowadays, that is by specifying a list of ranges. So that you can use something like -opt-bisect=1-1000
to get something similar to the current opt-bisect-limit, but also -opt-bisect=1-10,20-30
to skip a certain range of passes, etc.
@Artem-B @nikic when you have a chance please look over the newest version of this. Here are the updates:
|
llvm/include/llvm/Support/Range.h
Outdated
int64_t Begin; | ||
int64_t End; | ||
|
||
Range(const int64_t Begin, const int64_t End) : Begin(Begin), End(End) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Range(const int64_t Begin, const int64_t End) : Begin(Begin), End(End) {} | |
Range(int64_t Begin, int64_t End) : Begin(Begin), End(End) {} |
Here and elsewhere, drop the const on non-reference/pointer params.
llvm/include/llvm/Support/Range.h
Outdated
} | ||
|
||
/// Get the size of this range | ||
int64_t size() const { return End - Begin + 1; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should return an unsigned type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. I also realized that End - Begin can overflow. I don't know if what I implemented is overkill, but I made it safer so that it won't overflow.
llvm/include/llvm/Support/Range.h
Outdated
} | ||
}; | ||
|
||
/// Utility class for parsing and managing range specifications |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Utility class for parsing and managing range specifications | |
/// Utility class for parsing and managing range specifications. |
Here an elsewhere, end sentences with a period, including in doc comments.
llvm/include/llvm/Support/Range.h
Outdated
const char Separator = ','); | ||
|
||
/// Legacy interface for backward compatibility. | ||
/// \deprecated Use the Expected<RangeList> version instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a brand-new API, why have a deprecated interface?
llvm/lib/Support/Range.cpp
Outdated
|
||
// Split by the specified separator | ||
SmallVector<StringRef, 8> Parts; | ||
Str.split(Parts, Separator, -1, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an iterator variant of this, no need to collect into a vector.
llvm/lib/Support/Range.cpp
Outdated
Ranges.push_back(Range(Begin, End)); | ||
} | ||
|
||
return std::move(Ranges); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't need std::move here due to NRVO?
llvm/lib/Support/Range.cpp
Outdated
} | ||
|
||
std::string RangeUtils::rangesToString(const ArrayRef<Range> Ranges, | ||
const char Separator) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the to string functionality used for anything but testing? If not, we should only support the raw_ostream interface.
llvm/lib/IR/OptBisect.cpp
Outdated
} else if (Limit > 0) { | ||
// Convert limit to range 1-Limit | ||
std::string RangeStr = Limit == 1 ? "1" : "1-" + llvm::utostr(Limit); | ||
auto Ranges = RangeUtils::parseRanges(RangeStr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going through a string and then parsing it here is redundant? You can directly create the range.
llvm/test/Other/opt-bisect-ranges.ll
Outdated
; Test that verifies functionality for -opt-bisect with range specifications | ||
|
||
; Test basic range functionality: run passes 1-3 and 7-8 | ||
; RUN: opt -O1 -opt-bisect=1-3,7-8 %s 2>&1 | FileCheck %s --check-prefix=CHECK-RANGES |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to replace -O1
here with an explicit -passes
list. This will save some poor person from updating this test when the O1 optimization pipeline changes.
Hey @nikic I think I addressed all your comments, thanks for the feedback :) The most major change is I decided to make Range into a class to preserve the Begin <= End invariant. |
Add -opt-bisect-skip=<idx> option for skipping pass at index <idx> in the pipeline.
OptBisect allows you to binary search for a pass causing some error. It is useful as a next step to disable that pass as a sanity check and to see if the issue is isolated to one pass.